Skip to content

fix: delete card confirmation + column Enter key submit (#513 #516)#527

Merged
Chris0Jeky merged 4 commits intomainfrom
fix/513-516-ux-small-fixes
Mar 29, 2026
Merged

fix: delete card confirmation + column Enter key submit (#513 #516)#527
Chris0Jeky merged 4 commits intomainfrom
fix/513-516-ux-small-fixes

Conversation

@Chris0Jeky
Copy link
Copy Markdown
Owner

Summary

Two small UX fixes discovered in the 2026-03-29 manual testing session.

Fix 1: Delete Card confirmation dialog (#513)

  • Clicking "Delete Card" now shows a TdDialog confirmation before proceeding
  • Prevents accidental irreversible card deletion
  • Dialog shows the card name for context, has Cancel + Delete buttons
  • Delete button shows "Deleting…" and is disabled during the async call to prevent double-submit

Fix 2: Column name Enter key (#516)

  • Pressing Enter in the column name input now submits the form
  • Added @keydown.enter.prevent to stop the event from propagating to the board beneath

Test changes

  • Updated 2 CardModal tests that previously spied on window.confirm to instead drive the new TdDialog flow

Closes

Closes #513, Closes #516

Risk

Low — UI-only changes; no store or API changes.

Replace browser-native confirm() with TdDialog in CardModal. The dialog
shows the card name, prevents double-submit via isDeleting guard, and
is cancellable via Cancel button or backdrop click. Updates tests to
drive the new dialog flow instead of spying on window.confirm.
Add @keydown.enter.prevent on the column name input so pressing Enter
submits the column creation form instead of bubbling to the board canvas
underneath.
@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

Ensures isDeleting is always cleared after the delete attempt, whether
it succeeds or fails, guarding against stuck disabled states.
@Chris0Jeky
Copy link
Copy Markdown
Owner Author

Self-Review (Adversarial Pass)

#513 checks:

  • Confirmation is cancellable: Yes — Cancel button calls handleDeleteCancel setting showDeleteConfirm = false. Backdrop click also cancels (unless delete is in progress, where closeOnBackdrop is bound to !isDeleting).
  • Card name shown in dialog for context: Yes — description prop interpolates card.title directly.
  • No double-submit path: Yes — isDeleting guard at top of handleDeleteConfirm returns early if already deleting. Both buttons are disabled during the async call. Guard was also tightened to use finally so isDeleting always resets regardless of success or failure.
  • isDeleting reset: Fixed in follow-up commit — finally block ensures isDeleting = false is always reached, preventing any stuck-disabled state.

#516 checks:

  • Enter only triggers submit, not board interaction: Yes — @keydown.enter.prevent both calls createColumn and calls .preventDefault(), stopping propagation to the board canvas beneath.
  • Empty-name validation handled correctly: Yes — createColumn already guards with if (!newColumnName.value.trim()) return at line 137 of BoardView.vue, so pressing Enter on an empty field is a no-op.

Additional observations:

  • No store or API surface changed — risk is confined to UI layer.
  • TdDialog is already used in other modal contexts (TdDialog.spec.ts confirms the component is well-tested). The z-index of td-dialog-backdrop (z-60) correctly sits above the card modal (z-50).
  • Tests updated from window.confirm spy to driving the actual TdDialog DOM, which gives better coverage of the real user interaction.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces the native browser confirmation dialog for card deletion with a custom TdDialog component in CardModal.vue, adding state management to handle the deletion process and updating the corresponding tests. It also adds an enter key handler to the column creation input in BoardView.vue. Feedback suggests scoping DOM queries in tests to the dialog for better reliability and identifies a redundant event handler in the column creation form.

Comment on lines +335 to +337
const confirmButton = Array.from(document.querySelectorAll<HTMLButtonElement>('button')).find(
(btn) => btn.textContent?.trim() === 'Delete',
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation to find the confirmation button queries the entire document, which could lead to flaky tests if another button with the text 'Delete' is present. It's better to scope the query to the dialog element to make the test more robust. It would also be good practice to assert that dialogElement is not null before using it.

Suggested change
const confirmButton = Array.from(document.querySelectorAll<HTMLButtonElement>('button')).find(
(btn) => btn.textContent?.trim() === 'Delete',
)
const dialogElement = document.querySelector('.td-dialog');
const confirmButton = Array.from(dialogElement!.querySelectorAll<HTMLButtonElement>('button')).find(
(btn) => btn.textContent?.trim() === 'Delete',
);

placeholder="Column name"
class="td-column-form__input"
autofocus
@keydown.enter.prevent="createColumn"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The @keydown.enter.prevent handler on this input is redundant. The parent <form> element already has a @submit.prevent="createColumn" handler. Standard browser behavior will trigger the form's submit event when Enter is pressed in a text input, so the existing form handler is sufficient. Removing this duplicate handler simplifies the code and relies on idiomatic form handling.

Use a computed property for the dialog description so the card title
is wrapped in real quote characters instead of the HTML entity &quot;
that was leaking through into the text-interpolated paragraph.
@Chris0Jeky
Copy link
Copy Markdown
Owner Author

Adversarial Review (Independent Pass)

Verdict: REQUEST CHANGES (one bug fixed inline — see below)


Findings

MINOR (fixed in this review pass): &quot; HTML entity leak in delete dialog description

The :description prop was set with a Vue template backtick expression containing &quot;:

:description="`Are you sure you want to delete &quot;${card.title}&quot;? ...`"

In a JavaScript template literal, &quot; is NOT an HTML entity — it is 6 literal characters. TdDialog renders the prop via {{ props.description }} (text interpolation, no HTML decoding), so the user would see:

Are you sure you want to delete "My Card"? This action cannot be undone.

Fix applied: Introduced a deleteConfirmDescription computed property in the script block using real quote characters, and bound :description="deleteConfirmDescription" in the template. Commit d335b5ac pushed to this branch.


#513 checks

  • Double-delete protected (button disabled during async): passisDeleting guard at top of handleDeleteConfirm plus :disabled="isDeleting" on the button
  • Escape/backdrop safe during in-flight delete: passclose-on-backdrop="!isDeleting" blocks backdrop; Escape stack is LIFO so TdDialog's handler fires first and calls handleDeleteCancel, which only sets showDeleteConfirm = false (does not emit close on CardModal); isDeleting stays true so no state corruption
  • Error path leaves card intact: passshowDeleteConfirm is NOT closed on error (dialog stays open for retry), isDeleting resets via finally. Minor note: no error toast is shown to the user — a silent console.error only — but that's pre-existing behaviour and out of scope
  • Card name shown in dialog: pass — title shown in description (once the &quot; bug is fixed)

#516 checks

  • Empty column name guarded: passcreateColumn() has if (!newColumnName.value.trim()) return before the API call
  • .prevent stops propagation correctly: pass@keydown.enter.prevent both prevents default (form submit firing twice since @submit.prevent is also present) and stops the event reaching board-level keyboard shortcuts
  • Other similar forms noted (out of scope here): notedColumnEditModal name input has no @keydown.enter handler, but it is a full modal with a Save button and no board-level Enter shortcut in scope, so the missing binding is not a regression. ColumnLane inline card form uses a <textarea> where Enter is intentionally a newline — correct as-is.

Test quality

  • Tests drive real TdDialog DOM: pass — both updated tests use attachTo: document.body and query document.querySelector('.td-dialog') / document.querySelectorAll('button') to interact with the Teleport-rendered dialog, not a mock

Build

  • TypeScript clean: pass (120 files, 1102 tests, 0 errors after fix)
  • Tests pass: pass

Summary: One real bug (visible &quot; characters in the confirmation prompt) caught and fixed. All safety mechanisms for double-delete, escape handling, and error recovery are correct. #516 Enter-key fix is correct and well-scoped. Ready to merge after CI passes.

@Chris0Jeky
Copy link
Copy Markdown
Owner Author

Second Adversarial Review (Post-Fix Pass)

Verdict: APPROVE

Previous fix verified

  • deleteConfirmDescription computed uses real " quote characters (not &quot; HTML entities): verified correct — the computed property uses a JS template literal with literal " characters; TdDialog renders via {{ props.description }} text interpolation so users see proper quotes.

#513 checks

Card title displayed correctly in dialog: pass — deleteConfirmDescription computed produces e.g. Are you sure you want to delete "My Card"? This action cannot be undone. Note: if a card title itself contains ", the resulting string would read "My "Quoted" Card" — cosmetically awkward but not a safety issue and not introduced by this PR.

Double-delete protected: pass — if (isDeleting.value) return guard at the top of handleDeleteConfirm + :disabled="isDeleting" on both footer buttons. Even a programmatic .click() on the button is blocked by the guard.

Escape/backdrop safe during in-flight delete: pass — Backdrop click is blocked by :close-on-backdrop="!isDeleting". Escape key: the useEscapeStack is LIFO; when the delete dialog is open, TdDialog.requestClose sits on top of CardModal.handleClose. Escape during an in-flight delete triggers requestClose → emit('close') → handleDeleteCancel() which sets showDeleteConfirm = false (dialog closes visually) but does NOT emit close on CardModal — the modal stays open, the delete continues in the background, and isDeleting resets via finally. The card gets deleted silently without further UX feedback. This is safe and acceptable (delete cannot be cancelled once started), though slightly confusing UX. Pre-existing pattern consistent with the rest of the codebase; not a blocker.

Error path leaves card intact: pass — showDeleteConfirm is not closed in the catch block (dialog stays open for retry). isDeleting always resets via finally. The failed delete leaves the card untouched. Note: error is only console.error; no user-visible toast. Pre-existing behavior, out of scope.

Delete button visually distinct from Cancel: pass — Cancel is text-gray-700 border-gray-300 (neutral); Delete is text-white bg-red-600 hover:bg-red-700 (solid red). Clearly differentiated.


#516 checks

Enter binding on correct element: pass — @keydown.enter.prevent is on the <input> inside the <form @submit.prevent="createColumn">. Correct placement; the composable keyboard shortcut for Enter (which calls openSelectedCard) is only active when focus is not captured by an input, so no conflict.

Empty name guarded: pass — createColumn() has if (!newColumnName.value.trim()) return before the API call; pressing Enter on an empty field is a no-op.

Propagation stopped correctly: pass — .prevent calls event.preventDefault() on keydown, suppressing the synthetic form submit that browsers issue when Enter is pressed in a single-line input. The board-level useKeyboardShortcuts Enter handler is registered via window.addEventListener in bubble phase; the escape stack uses capture phase but trapFocus inside TdDialog only intercepts Tab. No double-fire.


Build

  • TypeScript clean: pass — vue-tsc -b exits 0, no errors
  • Tests pass: pass — 1102/1102 tests across 120 test files

Additional findings

Minor / informational — out of scope:

  1. handleDeleteComment (comment-level delete) still uses window.confirm(). This pre-existed and BUG: Delete Card has no confirmation dialog — immediate irreversible deletion #513 was scoped only to card-level deletion. A follow-up issue would be appropriate to unify the confirmation pattern.
  2. createColumn has no in-flight guard (no isCreating flag), so rapid double Enter could submit twice. This is a pre-existing risk identical to clicking the Create button twice; not introduced by this PR.

None of the above are blockers for merging this PR.

@Chris0Jeky Chris0Jeky merged commit bac0cfd into main Mar 29, 2026
18 checks passed
@Chris0Jeky Chris0Jeky deleted the fix/513-516-ux-small-fixes branch March 29, 2026 14:51
@github-project-automation github-project-automation bot moved this from Pending to Done in Taskdeck Execution Mar 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

1 participant